Add UseConsistentParameterSetName Rule#2124
Add UseConsistentParameterSetName Rule#2124liamjpeters wants to merge 8 commits intoPowerShell:mainfrom
Conversation
liamjpeters
requested review from
andyleejordan,
bergmeister,
michaeltlombardi and
sdwheeler
as code owners
| 1. **Missing DefaultParameterSetName** - Warns when parameter sets are used but no default is specified | ||
| 2. **Multiple parameter declarations** - Detects when a parameter is declared multiple times in the same parameter set. This is ultimately a runtime exception - this check helps catch it sooner. | ||
| 3. **Case mismatch between DefaultParameterSetName and ParameterSetName** - Ensures consistent casing | ||
| 4. **Case mismatch between different ParameterSetName values** - Ensures all references to the same parameter set use identical casing | ||
| 5. **Parameter set names containing newlines** - Warns against using newline characters in parameter set names |
There was a problem hiding this comment.
We need to verify our understanding of parameter sets to review this better, we'll get back to it Liam!
|
@liamjpeters can you resolve merge conflict please so we can run CI? |
Done. If you approve the CI runs, I'll fix any issues that may have cropped up |
Thanks for quick turnaround, approved just now. You should be able to run the tests in your fork as well btw (at least that was the case when we were in Azure DevOps before we moved CI to GitHub). If not, let me know, we can look into enabling that generally or just for you as a regular contributor |
The triggers for the CI are a push or PR to |
I just updated default branch in my fork to main, you go to settings and rename it here and it then even gives you instructions how to update your local clone, if you didn't want to re-clone. But it's not required for your fork in order to run tests or sync, I've done it with master until now. In the meantime you should be able to run tests for future work by opening PR in your fork against your fork's default branch but I opened a PR to add |
| // consistency | ||
| var duplicateAttributes = paramBlockInfo | ||
| .GroupBy(p => new { p.ParameterName, p.ParameterSetName }) | ||
| .Where(g => g.Count() > 1) |
There was a problem hiding this comment.
Using this has better performance as functions ends once it finds first element instead of enumerating all of them
| .Where(g => g.Count() > 1) | |
| .Where(g => g.Skip(1).Any()) |
There was a problem hiding this comment.
I think in this case Count() > 1 is actually faster and easier to read.
The g here is a Grouping which implements IList and subsequently ICollection - which has a count property. The linq Count() uses this count rather than iterating when it can.
I made an attempt at benchmarking the difference (I've only done a little C# benchmarking, so this was LLM-assisted). There's not a huge difference in it, but count certainly appears to be faster and allocates a little bit less.
Both are dwarfed by the Grouping operation though.
Co-authored-by: Christoph Bergmeister <c.bergmeister@gmail.com>
3 participants
PR Summary
Add UseConsistentParameterSetName rule to detect potential parameter set issues
Parameter set names are case-sensitive in PowerShell (unlike most other elements), which can lead to runtime errors and unexpected behavior when not handled correctly.
The rule performs five key validations:
Fixes #396 with the exception of
ParameterSets that don't have at least one unique parameter.This won't help with Dynamic Parameters.
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.